fix: add managemement command to filter hook by ktyagiapphelix2u · Pull Request #200 · edx/edx-platform

ktyagiapphelix2u · 2026-03-26T12:47:00Z

Description

Add a pluggable filter hook in manage.py so Django management command execution can be intercepted by configured Open edX filter pipelines. This keeps edx-platform limited to the public integration point and avoids embedding org-specific monitoring logic here

Reference Ticket

https://2u-internal.atlassian.net/browse/BOMS-151

Copilot

Pull request overview

Adds an Open edX Filters integration point to manage.py so Django management command execution can be intercepted (e.g., wrapped/monitored) via configured filter pipelines, keeping org-specific logic out of edx-platform.

Changes:

Introduces a new public filter type (org.openedx.platform.management.command.execute.requested.v1) for management command execution.
Wraps execute_from_command_line in a command_runner callable and passes it through the filter pipeline before execution.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

manage.py:135

OpenEdxPublicFilter.run_pipeline() appears to return a tuple of the filtered values (as used elsewhere in this repo via tuple-unpacking of *.run_filter(...)). Treating the result as a dict (pipeline_output.get(...)) will raise AttributeError and cause the broad except to always fall back to command_runner, effectively preventing the filter pipeline from ever overriding the runner.

        pipeline_output = ManagementCommandExecutionRequested.run_filter(
            command_name=command_name,
            service_variant=os.environ.get("SERVICE_VARIANT", edx_args.service_variant),
            command_runner=command_runner,
        )

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

pwnage101 · 2026-07-27T18:15:32Z

+    def command_runner():
+        return execute_from_command_line([sys.argv[0]] + django_args)
+
+    command_name = next((arg for arg in django_args if not arg.startswith('-')), 'help')


IMO, this method of calculating the command name is obscure and confusing. At least add a code comment explaining the rationale, and why the default value of "help" makes sense. Give a realistic example value of django_args and what the resulting output is supposed to be for that input value.

I added an explicit comment explaining how command_name is derived (first non-option token from django_args), why the fallback is help, and a concrete example input/output (for example: --verbosity 2 migrate --noinput -> migrate; --help -> help). This should make the intent clearer for future readers.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

manage.py:138

The current command name extraction will incorrectly pick option values as the command. For example, with django_args=['--verbosity', '2', 'migrate', '--noinput'] this code returns '2', not 'migrate', because it selects the first token that doesn't start with '-'. This will cause the filter pipeline to receive the wrong command_name for common Django global options that take values.

    # We treat the first non-option token as the Django command name.
    # Example: django_args=['--verbosity', '2', 'migrate', '--noinput'] -> 'migrate'.
    # If there is no non-option token (for example, django_args=['--help']),
    # default to 'help' because Django will print command help in that case.
    command_name = next((arg for arg in django_args if not arg.startswith('-')), 'help')

manage.py:30

Class name uses Contextmanager rather than the standard ContextManager CamelCase, which makes the name harder to read and inconsistent with typical Python naming for context managers.

class ManagementCommandContextmanagerRequested(OpenEdxPublicFilter):

manage.py:140

If the class is renamed to ManagementCommandContextManagerRequested, the call site also needs to be updated to match; otherwise this will raise a NameError at runtime.

    command_contextmanager = ManagementCommandContextmanagerRequested.run_filter(

pwnage101 · 2026-07-29T18:20:04Z

+    command_contextmanager = ManagementCommandContextmanagerRequested.run_filter(
+        command_contextmanager=nullcontext(),
+        command_name=command_name,
+        service_variant=os.environ.get("SERVICE_VARIANT", edx_args.service_variant),
+    )


Output signature should match input signature:

Suggested change

command_contextmanager = ManagementCommandContextmanagerRequested.run_filter(

command_contextmanager=nullcontext(),

command_name=command_name,

service_variant=os.environ.get("SERVICE_VARIANT", edx_args.service_variant),

)

command_contextmanager, _, _ = ManagementCommandContextmanagerRequested.run_filter(

command_contextmanager=nullcontext(),

command_name=command_name,

service_variant=os.environ.get("SERVICE_VARIANT", edx_args.service_variant),

)

pwnage101 · 2026-07-29T18:31:08Z

+from openedx_filters.tooling import OpenEdxPublicFilter
+
+
+class ManagementCommandContextmanagerRequested(OpenEdxPublicFilter):


This class is going to be deleted, right? It's just here temporarily to make the tests pass?

Yes, this is only a temporary workaround to get the tests passing. Once openedx-filters PR is merged and released, we'll update the openedx-filters dependency in edx-platform's requirements and remove the fallback class definition from manage.py.

pwnage101 · 2026-07-29T18:34:17Z

I also just wanted to make sure you plan to add this filter call upstream to openedx/openedx-platform

robrap · 2026-07-29T19:46:27Z

+    # django_args contains only args that argparse did not consume.
+    # Django treats the first positional token as the command name.
+    # Example: django_args=['migrate', '--noinput'] -> 'migrate'.
+    # If the first token is an option (for example, django_args=['--help']),
+    # default to 'help' so the filter sees a command-like label.
+    command_name = django_args[0] if django_args and not django_args[0].startswith('-') else 'help'


Would --help be an illegal name for command_name? Why not just make this django_args[0], with maybe a default of no-arg-supplied if there are no arguments?

+1, it does feel like falling back to help can be misleading.

@robrap @pwnage101 The command_name extraction in manage.py now uses django_args[0] directly, allowing option flags such as --help to be passed through unchanged instead of being mapped to help. If no arguments are provided, it defaults to no-argument.

ktyagiapphelix2u · 2026-07-30T06:33:59Z

class ManagementCommandContextmanagerRequested(OpenEdxPublicFilter):

@pwnage101 Yes, I'm going to create an upstream PR. Once this PR is approved, I'll close it, open a new PR upstream, get that merged, and then cherry-pick the changes back into this Fork.

ktyagiapphelix2u added 3 commits March 26, 2026 12:39

fix: add managemement command to filter hook

27a8b2e

fix: add managemement command to filter hook

e0628c6

fix: add managemement command to filter hook

8e3ba1d

ktyagiapphelix2u marked this pull request as ready for review July 14, 2026 13:37

Copilot AI review requested due to automatic review settings July 14, 2026 13:37

Copilot started reviewing on behalf of ktyagiapphelix2u July 14, 2026 13:38 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Comment thread manage.py Outdated

Comment thread manage.py Outdated

fix: add managemement command to filter hook

7503829

Copilot AI review requested due to automatic review settings July 20, 2026 09:30

Copilot started reviewing on behalf of ktyagiapphelix2u July 20, 2026 09:31 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

ttak-apphelix reviewed Jul 22, 2026

View reviewed changes

Comment thread manage.py Outdated

fix: add managemement command to filter hook

381a748

Copilot AI review requested due to automatic review settings July 23, 2026 05:13

Copilot started reviewing on behalf of ktyagiapphelix2u July 23, 2026 05:13 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread manage.py

Comment thread manage.py Outdated

pwnage101 requested changes Jul 27, 2026

View reviewed changes

Comment thread manage.py Outdated

pwnage101 reviewed Jul 27, 2026

View reviewed changes

pwnage101 mentioned this pull request Jul 27, 2026

feat: add contextmanager-based management command observability filter openedx/openedx-filters#344

Open

fix: add managemement command to filter hook

58afa12

Copilot AI review requested due to automatic review settings July 28, 2026 06:29

Copilot started reviewing on behalf of ktyagiapphelix2u July 28, 2026 06:29 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

ktyagiapphelix2u requested a review from pwnage101 July 28, 2026 08:02

pwnage101 reviewed Jul 28, 2026

View reviewed changes

Comment thread manage.py Outdated

fix: add managemement command to filter hook

ff6df4b

pwnage101 reviewed Jul 29, 2026

View reviewed changes

robrap reviewed Jul 29, 2026

View reviewed changes

ktyagiapphelix2u added 2 commits July 30, 2026 06:07

fix: add managemement command to filter hook

b9d910f

fix: add managemement command to filter hook

09956fd

ktyagiapphelix2u requested review from pwnage101 and robrap July 30, 2026 06:36

		from openedx_filters.tooling import OpenEdxPublicFilter


		class ManagementCommandContextmanagerRequested(OpenEdxPublicFilter):

Uh oh!

Conversation

ktyagiapphelix2u commented Mar 26, 2026

Description

Reference Ticket

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwnage101 Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwnage101 commented Jul 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktyagiapphelix2u commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pwnage101 Jul 29, 2026 •

edited

Loading